Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: SafeErc20 utility #289

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

feat: SafeErc20 utility #289

wants to merge 35 commits into from
+738 −1

Conversation

0xNeshi
Copy link

@0xNeshi 0xNeshi commented Sep 17, 2024 •
edited
Loading

Resolves #279

NOTE: It is possible I misused or misunderstood certain functionality of the Stylus SDK or just how to write Stylus contracts in general. If that is the case, it goes without saying that, for the sake of getting up to speed with Stylus, I would greatly appreciate any feedback on how to write contracts better (or just correctly).

I was torn between making SafeErc20 an inheritable contract and making it a trait implemented against ERC20 tokens, but I opted for the latter as it seems to more closely fit the "this is an extension of the ERC20" paradigm. In other words, if devs need to have the "safe" functions, they can import the trait and use the functions as regular ERC20 ones.

PR Checklist

  • Tests
  • Documentation

@netlify Netlify
Copy link

netlify bot commented Sep 17, 2024 •
edited
Loading

✅ Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit bf1915a
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/66fa668b5f0f06000814cb83

@0xNeshi 0xNeshi mentioned this pull request Sep 17, 2024
Open
1 task
@0xNeshi 0xNeshi marked this pull request as draft September 17, 2024 14:38
qalisander
qalisander reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@qalisander qalisander left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @0xNeshi and thanks for your contribution!
Appreciate it!

Hard to tell for me right now what kind of design should be the best for SafeErc20 with current state of the stylus-sdk.

IMHO we can try to build a working example first with end-to-end tests. Then later dive into optimization if it will make sense.
There is no good interoperability between rust struct, that defines a contract in stylus and an external call to that contract (deployed at some address) as it is in solidity.
So we can try to add Erc20 solidity abi into sol_interface! macro like IERC721Receiver example.
And use call contract's type generated by sol_interface! as a first argument like it is in solidity SafeErc20 example.
But we should process boolean responses from underlying Erc20 contract differently as it is specified in the solidity counterpart.

Then we can use IErc20 trait to create wrapped Erc20 contract. It should route most calls externally using the Call::new(..) api from the stylus-sdk (like Erc721Receiver example)

How do you feel about this plan?

@0xNeshi
Copy link
Author

0xNeshi commented Sep 19, 2024 •
edited
Loading

EDIT: I realized what the issue is, see #289 (comment)

@qalisander sounds like a good plan!

I implemented what I understood to be what you had imagined. See the most recent changes.
Basically, I updated SafeErc20 to be an inheritable contract, where its safe_X functions take the token address as the first argument (thanks for the tip).

Note: I'm trying to implement a working solution for the regular ERC20 transfer function (i.e. working on SafeERC20's safe_transfer function), which returns a bool. Once successful, I will handle the general case where the function might return a random type or not even return anything (e.g. USDT). After that, implementing other safe_X functions should be a piece of cake.

I also implemented one "happy flow" test for safe_transfer, but I have been banging my head against one problem for the last 6 hours though. It seems that calling the function like erc20.transfer(call, to, value) doesn't pass the expected msg sender. In other words, ERC20's msg.sender does not seem to be the same as SafeErc20's msg::sender() when invoked from within the SafeErc20 contract.

For context, I implemented a mock ERC20 contract that on calling transfer just updates the balances as expected (w/o checks), i.e. it reduces the amount from msg.sender and increases it for to (address). But the test keeps failing as just calling the safe_transfer function reverts immediately and I managed to narrow the revert error down to the type of stylus_sdk::call::Error::Reverted (it's a bit difficult to debug Stylus contracts for an amateur, maybe it's just a skill issue).

To test my hypothesis that msg.sender within ERC20 is not the same as msg::sender() within SafeErc20, I changed mock ERC20's transfer function to accept an address from argument (basically turning it into transferFrom), and passed msg::sender() as the value for this argument from within SafeErc20 like erc20.transfer(call, msg::sender(), to, value) - after this the test started passing!

I tried another approach by using RawCall::new(), passing the encoded calldata and invoking the ERC20 contract that way, but the result was the same (a failing test).

Might you have an idea what the problem might be here? Is this an SDK issue or have I misconfigured something?

0xNeshi
0xNeshi commented Sep 23, 2024
View reviewed changes
scripts/e2e-tests.sh Outdated
@@ -11,4 +11,4 @@ cargo +"$NIGHTLY_TOOLCHAIN" build --release --target wasm32-unknown-unknown -Z b
export RPC_URL=http://localhost:8547
# We should use stable here once nitro-testnode is updated and the contracts fit
# the size limit. Work tracked [here](https://github.com/OpenZeppelin/rust-contracts-stylus/issues/87)
cargo +"$NIGHTLY_TOOLCHAIN" test --features std,e2e --test "*"
cargo +"$NIGHTLY_TOOLCHAIN" test --features e2e -p safe-erc20-example --test safe-erc20
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will revert this once ready for merge, using this for easier testing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. If yours IDE allows to set build settings for tests, you can also run specific integration test from IDE's UI.

Copy link
Author

@0xNeshi 0xNeshi Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the msg sender issue above, I hope the explanation was clear enough. The same issue is there for safe_transfer_from, as the msg sender isn't the one expected (the original sender of tx).

EDIT: I realized what the issue is, see #289 (comment)

@codecov Codecov
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 109 lines in your changes missing coverage. Please review.

Project coverage is 80.6%. Comparing base (8fb2758) to head (d47db5a).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
contracts/src/token/erc20/utils/safe_erc20.rs 0.0% 109 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
contracts/src/token/erc20/mod.rs 84.0% <ø> (ø)
contracts/src/token/erc20/utils/safe_erc20.rs 0.0% <0.0%> (ø)

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bidzyyys bidzyyys Awaiting requested review from bidzyyys bidzyyys is a code owner

@qalisander qalisander Awaiting requested review from qalisander qalisander is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: SafeERC20 utility
2 participants
@0xNeshi @qalisander